Rework group and remove fail #126
Conversation
|
@sjakobi let me know how this performs on your benchmarks, I am quite curious to see if this is an improvement :) |
|
Impressive! Unfortunately this broke a bunch of formatting tests in Not all of them look obviously worse, but some do. For example: You can run these yourself if you want to give them a try. I use this patch to use my local --- a/cabal.project
+++ b/cabal.project
@@ -1,4 +1,4 @@
-packages: ./dhall ./dhall-bash ./dhall-json ./dhall-yaml ./dhall-lsp-server ./dhall-nix
+packages: ./dhall ./dhall-bash ./dhall-json ./dhall-yaml ./dhall-lsp-server ../prettyprinter/prettyprinter ../prettyprinter/prettyprinter-ansi-terminal
-- TODO: Remove this once hnix has a compatible release:
-- https://github.com/haskell-nix/hnix/issues/524The command I use is |
|
Woo walls of errors ^^ I'll look through them some more :) Thanks for sharing. |
Yep. A more robust testsuite is the least that we ought to get out of this experiment! 👍 |
|
I could not get the dhall testsuite running (haven't tried much either because my property tests were failing in the kotlin project and that gave nice examples). So this fails for several things: I eventually got it to pass tests, however not without complicating the whole thing, will translate it to haskell and push something tomorrow, don't have much time for it today... So a few interesting points here:
So in conclusion I think this idea is worth exploring further because it produces very small documents and avoids Fail as much as possible. If I understand this correctly every step taken in this function would have also been taken by For now if you want to take a look at the kotlin code: (I had to recreate all that debug stuff you made a while ago to even be able to work with on this ^^) |
Can you expand on this? I'd very much like to understand it! I'd also like to understand how it fits together with this bit:
Thanks for the link to your kotlin code – TBH it's rather difficult for me to read though. :/
Note that |
|
On phone so text will be a little uglier, doing backticks etc is just too slow^^
Sure, let's quickly reiterate how this whole thing works:
Now onto the harder part: Checking what flattening results in: We get here from either FlatAlt or Cat.
The last part is flatten itself, but that is left unchanged from the current code.
Takes quite a bit of time to get used to ^^ Especially because this is by no means how one should write kotlin code, it's just hard to make a datatype like Doc stacksafe an efficient without it. (Before introducing all the Eval stuff it choked on garbage collection because the layouter had to fully build every doc into the full simpledoc every time)
True, I forgot! This will pose quite the penalty to it as it has to evaluate the rightmost path, evaluate all left paths. It is actually the only algorithm that benefits from having Union as high up as possible. This can't really be seperate from group because it assumes existing Unions to be optimal already. |
|
Thanks for your explanations! They make sense to me. Unfortunately I still don't understand what you meant with
But maybe I should just wait for you to fix this PR – the code might make it clearer. :) |
|
Here's an idea for testing the new |
|
Thats actually what I did ^-^ |
|
Put the updated thing on the branch. This fails 2 doctests for reflow, will check those later today or tomorrow. Interestingly the property |
And there it goes and fails straight away after just 19 tests with a huge counterexample... Edit: Nvm fixed, now its just back to doctests failing with imports missing. |
|
Nice! :) Could you possibly address the CI failures (mostly HLint I believe)? |
|
|
||
| Column f -> Flat (Column (flatten . f)) | ||
| Nesting f -> Flat (Nesting (flatten . f)) | ||
| WithPageWidth f -> Flat (WithPageWidth (flatten . f)) |
There was a problem hiding this comment.
So why do we need to use flatten instead of group here? I think you tried to explain that before but I haven't grokked it yet. ;)
There was a problem hiding this comment.
Because changesOnFlattening is expected to return a value that tells the caller if the doc changed (either by flattening, by encountering a Line or not changing). And you cannot get that information out of a function :/
There was a problem hiding this comment.
Hm, I don't understand what you're saying here. Both flatten and group return Docs. And group can actually add information by wrapping things in Union.
There was a problem hiding this comment.
Thats what you are thinking about! Yes definitly that is a good idea, I'll test that
There was a problem hiding this comment.
This fails quickcheck with a huge counterexample again and my own tests with a small one. Now to figure out whats wrong...
There was a problem hiding this comment.
Counterexample: column (\_ -> hardline). This flattens to column (\_ -> Fail) but when using group to column (\_ -> Line).
There was a problem hiding this comment.
Ah, that's very interesting! Would that explain the test failures I got for the first version of this PR?
There was a problem hiding this comment.
Yes, or at least the problem was very similar iirc.
| x@Char{} -> x | ||
| x@Text{} -> x | ||
|
|
||
| data FlatteningResult a |
There was a problem hiding this comment.
Does this type have different semantics than FlattenResult, or what's the reason for adding it?
If you don't like the constructor names on FlattenResult, feel free to change them. (I might bikeshed your suggestions a bit though. ;))
There was a problem hiding this comment.
Oh I just added names this way to not conflict with the others. No technically you could the other type, just wanted to make a clear distinction, if this ends up performing much better than master and you want to merge it, I can change that 👍
prettyprinter/test/Testsuite/Main.hs
Outdated
| groupedLayedOut = layout layouter grouped | ||
| groupedSimpleLayedOut = layout layouter groupedSimple | ||
| in counterexample ("Grouped: " ++ (show . diag) (grouped)) | ||
| (counterexample ("Grouped (Simple) " ++ (show . diag) (groupedSimple)) |
There was a problem hiding this comment.
This nested counterexample is a bit surprising IMHO – I'd prefer something similar to mkCounterexample above.
There was a problem hiding this comment.
^^ Agree, just took a shortcut tbh ;)
Co-Authored-By: Simon Jakobi <[email protected]>
Co-Authored-By: Simon Jakobi <[email protected]>
|
In my I'll check the benchmarks in this package too (or you can if you want). What I suspect might be the reason, is that by pushing the I have recorded one idea to counter this issue in #123 (comment). What happens when we wrap each |
This makes sense. This group implementation tries it's hardest to not produce branches, or produce very small branches, so subsequent group calls will suffer!
Putting it into |
Hm, that might cause more work for the layouters… If we can't find another marker, we could consider adding a dedicated constructor |
That was also initially my thought, this will happen if x evaluates to Fail. So |
Ah another case of doctests fail and the property tests succeed 😱 |
In |
Those are the import errors in the doctests. Theres far more things failing by inserting a |
Can you post them here? |
They are all pretty much the same: |
|
This is to blame: It incorrectly flattens |
|
Either way, I am out for today, will check on this tomorrow again^^ |
|
So let's reiterate a bit, because I think this approach is flawed:
So how would one improve this:
If Ideas/thoughts? Either way this experiment helped me understand how flattening works on a whole new level, so I'd already call it a win ^^ |
|
Thanks for the great summary, @1Jajen1! :) I think we should eventually put most of it into a note in the code. Regarding the idea to keep track of what's flat, I think the obvious solution is to add a new constructor |
|
I'll play around with adding a |
|
Sounds good to me, @1Jajen1! 👍 Let's also make sure that any lessons learned make it into documentation in the code where I think there's the highest probability that they will be preserved. BTW note that I've done a more conservative experiment with group at #140, although with good results so far. I haven't checked how much |
|
Been a while 😅 I did some work on the kotlin version and remembered this attempt and that I still have this note to write 🙈 Also when thinking about this again, this left me somewhat unsatisfied as this seems like an approach that could lead to better performance. Has anything changed on the benchmark side, that makes it a bit easier to just play around with a few ideas? |
There was a problem hiding this comment.
Thanks for following up on this thread, Jannis! :)
Unfortunately our whole conversation is paged out for me now – which hopefully means that some of my questions will be easy to answer! 😅
Has anything changed on the benchmark side, that makes it a bit easier to just play around with a few ideas?
Unfortunately not. Maybe I should just bite the bullet and add a little benchmark package to this repo that depends on dhall.
| @@ -630,6 +630,24 @@ group x = case x of | |||
| -- See https://github.com/quchen/prettyprinter/issues/22 for the corresponding | |||
| -- ticket. | |||
|
|
|||
| -- Note [Group: Optimial placement of Union and loss of information] | |||
There was a problem hiding this comment.
| -- Note [Group: Optimial placement of Union and loss of information] | |
| -- Note [Group: Optimal placement of Union and loss of information] |
| @@ -630,6 +630,24 @@ group x = case x of | |||
| -- See https://github.com/quchen/prettyprinter/issues/22 for the corresponding | |||
| -- ticket. | |||
|
|
|||
| -- Note [Group: Optimial placement of Union and loss of information] | |||
There was a problem hiding this comment.
Could you add a reference to this note next to the other note reference in group?
| -- This approach would not increase the cost of a call to group at all as | ||
| -- changesUponFlattening already traverses deep enough already, however, due to |
There was a problem hiding this comment.
This isn't very obvious. Wouldn't the new approach increase the "length" of the Doc that we need to rebuild on top of the new Union?!
There was a problem hiding this comment.
Ah, I forget the cost of recreating the Doc, that whole line is guesswork anyway and should probably be removed. I was only thinking in traversal depth.
| -- changesUponFlattening already traverses deep enough already, however, due to | ||
| -- an unrelated property of group, placing the resulting 'Union' further down or | ||
| -- not at all will harm subsequent calls to group. Currently when calling group | ||
| -- with a document that cannot be flattened or an already flat document the Doc | ||
| -- will not change at all. This looses information as subsequent calls to group | ||
| -- have to come to the same conclusion again and thus retraverse. If we now | ||
| -- place the Union (the only evidence that we have done some work) deeper in the | ||
| -- tree, subsequent calls will have to retraverse even more nodes. |
There was a problem hiding this comment.
Couldn't this extra work / loss of information be avoided by adding another Union Fail (or a new dedicated marker) on top of the result of group?
There was a problem hiding this comment.
That's what I was thinking as well. Even right now, without placing Unions deeper) doing so would avoid re-traversing failing or already flat documents. This + placing unions at the optimal depth could be interesting to play around with.
Related to #120
I may have overdone it by removing Fail entirely because one compat module still needs it to convert from an external doc. Other than that this passes the tests (3 doctests fail with missing imports so I don't think they are related)
I'll comment more on this if needed, but for now this should show the idea ^^
Either way don't merge this, this probably needs to be tested a bit more.